Walkthrough

Changes

Pattern anchoring validation

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested reviewers: robnester-rh, simonbaird

• X
• Mastodon
• Reddit
• LinkedIn

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Review — approve

PR: #1781 — Warn when regex patterns in rule data lack anchoring

Summary

This PR adds non-blocking warnings when user-supplied regex patterns in rule data lack ^ anchoring. OPA's regex.match performs substring matching, so unanchored patterns like console\.redhat\.com also match evil-console.redhat.com. The change is a well-targeted security improvement.

Changes reviewed across 9 files (346+/14−):

• policy/lib/sbom/sbom.rego — Four new rule_data_errors rules check anchoring for allowed_external_references, disallowed_external_references, allowed_package_sources, allowed_proxy_url_patterns, and disallowed_attributes except_when patterns.
• policy/release/buildah_build_task/buildah_build_task.rego — One new _rule_data_errors rule for disallowed_platform_patterns.
• policy/release/git_branch/git_branch.rego — New _rule_data_errors rule, new deny rule with proper METADATA annotations, and refactored matches_any to use a _branch_patterns_key constant.
• policy/release/rpm_build_deps/rpm_build_deps.rego — Same pattern as git_branch: new validation, new annotated deny rule, key constant refactoring.
• Test files — Comprehensive tests added for all anchoring checks, including positive/negative cases and updates to existing tests to account for new warnings.
• antora/docs/modules/ROOT/pages/authoring.adoc — Documentation updated with anchoring guidance.

Correctness

The implementation is sound. All anchoring checks follow the established rule_data_errors / _rule_data_errors pattern used throughout the codebase. The new rules emit "severity": "warning" so they are non-blocking. The METADATA annotations on the new deny rules in git_branch.rego and rpm_build_deps.rego correctly include short_name, failure_msg, and collections, following the required conventions.

The refactoring of hardcoded key strings into _branch_patterns_key and _sources_key constants is a clean improvement that keeps the code DRY.

Tests are comprehensive: each module has dedicated anchoring tests for both the unanchored (warning expected) and anchored (no warning) cases, plus existing validation tests are updated to include the new expected warnings.

Security

This is a net security improvement. The unanchored regex patterns it detects represent a real supply chain risk — malicious sources could bypass allowlists by registering domains that contain the allowed domain as a substring. The warnings guide policy authors toward safer patterns.

Style & conventions

The code follows existing codebase patterns faithfully. METADATA annotations are present and well-formed. The # regal ignore:line-length pragmas are used where needed. Test naming follows the test_<feature>_<scenario> convention.

Findings

Finding 1: The new anchoring check for external references (lines ~425–434 in the post-merge file) extracts the URL via object.get(ref, "url", "") and checks not startswith(pattern, "^"), but omits the is_string(pattern) guard that all three other new anchoring checks include (allowed_package_sources, allowed_proxy_url_patterns, disallowed_attributes except_when). While the JSON schema validation upstream ensures url is typed as string, adding the guard would maintain defensive consistency across all four rules. Low risk since schema validation catches type mismatches first, but worth aligning for maintainability.

Review — approve

Overall: This is a well-structured enhancement that adds non-blocking warnings when user-supplied regex patterns in rule data lack ^ anchoring — mitigating the real risk that OPA's regex.match performs substring matching and unanchored patterns silently match unintended values (e.g., console\.redhat\.com matching evil-console.redhat.com).

The implementation is consistent across all affected rule data keys, follows existing codebase patterns for rule_data_errors / _rule_data_errors validation, and includes thorough test coverage for both the warning and the anchored (no-warning) cases. The documentation addition in authoring.adoc is clear and actionable.

What was checked

• Correctness of anchoring checks: All five new rule_data_errors rules (external references, package sources, proxy URL patterns, except_when, platform patterns) correctly use not startswith(pattern, "^") and produce "severity": "warning" results. The is_string(pattern) guard prevents errors on non-string values.
• New deny rules in git_branch.rego and rpm_build_deps.rego: Both follow the established pattern from buildah_build_task.rego — _rule_data_errors set consumed by a deny rule with proper METADATA annotations (short_name, failure_msg: "%s", appropriate collections). The policy_data collection assignment is correct for rule data validation rules.
• Test coverage: Each rule data key has explicit tests for both unanchored (warning emitted) and anchored (no warning) patterns. Existing tests are correctly updated to expect the new warnings where test data uses unanchored patterns.
• Test data fix in git_branch_test.rego: The single_test_case mock pattern branch[0-9]+-rhel-[0-9]+.[0-9]+.[0-9]+$ is updated to ^.*branch[0-9]+-rhel-[0-9]+.[0-9]+.[0-9]+$. This preserves matching semantics (^.* at the start is equivalent to unanchored for regex.match) while avoiding spurious anchoring warnings in unrelated tests.
• Security: The warning severity (non-blocking) is appropriate for a change that could affect existing deployments. Users can migrate at their own pace without policy enforcement breaking.
• No PR body injection: The PR body contains only descriptive content and an external issue reference.

Findings

1. Missing make generate-docs output (low)

The PR adds two new METADATA-annotated deny rules (allowed_target_branch_patterns_format in git_branch.rego and allowed_rpm_build_dependency_sources_format in rpm_build_deps.rego). Per project conventions (AGENTS.md: "Run make generate-docs after changing policy annotations. Commit the regenerated files."), the generated Antora docs at antora/docs/modules/ROOT/pages/packages/release_git_branch.adoc and release_rpm_build_deps.adoc should be regenerated to include these new rule entries. The current docs only list the pre-existing rules. CI's make ci pipeline includes generate-docs and should flag this.

2. ^.* escape hatch does not improve anchoring (low)

The check only verifies that patterns start with ^. A user could suppress the warning by prepending ^.* to an existing unanchored pattern (e.g., ^.*console\.redhat\.com), which has identical matching behavior to the original unanchored pattern. This is an inherent limitation of a syntactic check. The warning message clearly guides toward proper anchoring ("Use anchored patterns to avoid unintended substring matches"), and patterns like ^.*suffix$ are legitimate for suffix matching, so stricter heuristics would produce false positives. This is a design trade-off, not a defect.

Review

Verdict: approve ✅

Summary

This PR adds non-blocking warning-severity rule_data_errors when user-supplied regex patterns in rule data lack ^ anchoring. This addresses a real security concern: OPA's regex.match performs substring matching by default, so an unanchored pattern like console\.redhat\.com also matches evil-console.redhat.com.

The changes are well-structured and consistent across all affected policy files:

Correctness

Pattern followed is sound. The new rules use "severity": "warning" with result_helper_with_severity, matching the established pattern in buildah_build_task.rego for non-blocking data validation warnings. Warnings go through deny (not warn) — this is correct per codebase convention; the framework distinguishes blocking vs. non-blocking via the severity field, not the rule name.

METADATA annotations are appropriate. The new rules in git_branch.rego and rpm_build_deps.rego include collections: [redhat_rpms, policy_data], which is consistent with existing data-validation rules.

Test coverage is comprehensive. Each new check has both positive (warning produced for unanchored patterns) and negative (no warning for ^-anchored patterns) test cases. Existing tests that use unanchored mock data are correctly updated to expect the new warnings.

Security

The PR correctly addresses the substring-matching risk. Using non-blocking warnings (rather than failures) is a good incremental approach that flags the issue without breaking existing configurations.

No new attack surfaces are introduced. PR body content is descriptive and contains no injection attempts.

Low-severity observations

1. Missing is_string guard on anchoring checks — In buildah_build_task.rego, the anchoring check iterates some r in rule_data.get(...) and calls not startswith(r, "^"). If r is a non-string (e.g., the integer 1 in the existing test data), startswith is undefined in Rego, not undefined evaluates to true, and the warning fires with a confusing message (Pattern '\x01' in ...). While the schema validation already catches type errors at failure severity, adding is_string(r) before the startswith check would avoid the noisy duplicate. The same pattern applies to the sbom.rego checks, though object.get(ref, "url", "") provides a string default that partially mitigates this. Non-blocking — the warning is harmless alongside the higher-severity type-error failure.

2. Test mock data uses ^.* prefix for mechanical anchoring — In git_branch_test.rego (line 20), the pattern changes from "branch[0-9]+-rhel-..." to "^.*branch[0-9]+-rhel-..." to suppress the new warning. While ^.* is semantically equivalent (matches the same strings as no anchor), this is a pragmatic and reasonable choice for test fixtures where the pattern intentionally matches mid-string content. No production impact.

Labels: PR adds policy validation warnings and updates documentation

Review of PR #1781 — Warn when regex patterns in rule data lack anchoring

Verdict: request-changes

The PR adds warning-level rule_data_errors checks for unanchored regex patterns across several rule data keys. The motivation is sound — OPA's regex.match performs substring matching, so console\.redhat\.com would match evil-console.redhat.com. The sbom.rego and buildah_build_task.rego integrations are correct. However, two of the four policy files have dead-code warnings that will never surface during policy evaluation.

Findings

🔴 High: rule_data_errors not wired into deny/warn in git_branch.rego

File: policy/release/git_branch/git_branch.rego

The PR adds a public rule_data_errors rule, but there is no deny or warn rule in this package that iterates over rule_data_errors. Compare with buildah_build_task.rego which has:

# METADATA annotation with short_name, failure_msg, collections...
deny contains result if {
    some error in _rule_data_errors
    result := metadata.result_helper_with_severity(rego.metadata.chain(), [error.message], error.severity)
}

git_branch.rego has no equivalent consumer. The rule_data_errors set is computed but never used during policy evaluation. The tests pass because they directly assert on git_branch.rule_data_errors, not through deny, masking the integration gap.

Additionally, the existing test fixture mock_rule_data already contains an unanchored pattern ("branch[0-9]+-rhel-[0-9]+.[0-9]+.[0-9]+$" — no ^ prefix), which would produce spurious warnings if rule_data_errors were wired into deny, breaking existing tests.

Remediation: Add a deny rule with proper METADATA annotations (including short_name, failure_msg: "%s", and relevant collections) that consumes rule_data_errors, following the pattern in buildah_build_task.rego. Update existing tests that use unanchored patterns in their fixtures.

🔴 High: rule_data_errors not wired into deny/warn in rpm_build_deps.rego

File: policy/release/rpm_build_deps/rpm_build_deps.rego

Same issue as git_branch.rego. The rule_data_errors rule is defined but no deny or warn rule consumes it. The existing package only exposes warn (not deny), so the anchoring warnings would never surface during evaluation. The tests pass because they directly check rpm_build_deps.rule_data_errors.

Remediation: Add a deny (or warn) rule with METADATA annotations that consumes rule_data_errors, consistent with how other packages wire up rule data validation.

🟡 Medium: Inconsistent rule_data_errors visibility convention

Files: policy/release/git_branch/git_branch.rego, policy/release/rpm_build_deps/rpm_build_deps.rego

The codebase convention for package-internal rule data validation uses _rule_data_errors (private, underscore-prefixed) consumed by a local deny rule — see buildah_build_task.rego, schedule.rego, cve.rego, etc. The PR uses rule_data_errors (public, no underscore) in git_branch.rego and rpm_build_deps.rego, but nothing imports these packages to consume the public rule. If the intent is internal consumption, use _rule_data_errors for consistency. If external consumption is intended, a consumer must be established.

Remediation: Rename to _rule_data_errors and add a local deny/warn consumer, consistent with the rest of the codebase.

🟢 Low: Tests verify rule internals rather than observable policy behavior

Files: policy/release/git_branch/git_branch_test.rego, policy/release/rpm_build_deps/rpm_build_deps_test.rego

The new tests for git_branch and rpm_build_deps directly assert on rule_data_errors rather than on deny or warn output. While unit-testing individual rules is valid, the absence of integration tests through deny/warn means the tests don't catch the wiring gap described above. The sbom_test.rego and buildah_build_task_test.rego tests correctly assert through deny, which is why those integrations are verified.

Remediation: After wiring rule_data_errors into deny/warn, add or update tests that assert through the public deny/warn interface.

What looks good

• policy/lib/sbom/sbom.rego: All four anchoring checks (external_references, package_sources, proxy_url_patterns, disallowed_attributes except_when) are correctly implemented and properly consumed through the existing deny rule in policy/release/sbom/sbom.rego via lib.sbom.rule_data_errors. Good coverage of different data shapes.
• policy/release/buildah_build_task/buildah_build_task.rego: The _rule_data_errors addition follows the established pattern correctly — private rule, consumed by the existing annotated deny rule. Tests updated appropriately.
• antora/docs/modules/ROOT/pages/authoring.adoc: Clear documentation of the anchoring requirement with a concrete example.
• Test coverage for the working integrations (sbom_test.rego, buildah_build_task_test.rego): Thorough positive and negative test cases.